Fix append processor ignore_empty_values edge case #136649
Merged
+260
−33
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug introduced by #105718
With
allow_duplicates: false
, we're pretty careful to not upcast a scalar into a list unless some kind of change has been made to the value -- that is, if you have a value of"2"
and you are ignoring duplicates and you try to append"2"
onto it, you still have a value of"2"
and not["2"]
.With #105718, there's a new behavioral path, that is
ignore_empty_values: true
. We need to take the same care here that we don't upcast a scalar (or the absence of a value) into a list unless there's actually some value to be appended. That is, if you haveignore_empty_values: true
and all the values that you want to append are actually empty, then the resulting operation should be a no-op, but it wasn't! This PR fixes that.ALSO, I'm super pleased with how
innerAppendValues
andinnerAppendValuesWithDuplicates
drop out because of this, I've never been especially happy with those methods existing. 😄Note: Given that there's another BC, I actually expect this to land in v9.2.0 (I imagine the labels will get corrected at some point).
Additional note: related to #104813, with was the feature request that asked for something like
ignore_empty_values
to begin with.